Implement sass --embedded in pure JS mode
#2413
Conversation
ntkme
force-pushed
the
embedded-compiler
branch
from
8d7d4de to
7084da7
Compare
ntkme
force-pushed
the
embedded-compiler
branch
2 times, most recently
from
e66df5b to
d53fcc5
Compare
ntkme
force-pushed
the
embedded-compiler
branch
9 times, most recently
from
d7e6206 to
b3794eb
Compare
ntkme
force-pushed
the
embedded-compiler
branch
3 times, most recently
from
9112b44 to
54fabf3
Compare
ntkme
force-pushed
the
embedded-compiler
branch
8 times, most recently
from
257b4fd to
ac718ee
Compare
| if (_gracefulShutdown) { | ||
| _channel.sink.close(); | ||
| } else { | ||
| exit(exitCode); | ||
| } |
There was a problem hiding this comment.
This is also changing the current behavior for the Dart VM.
There was a problem hiding this comment.
Actually it's not changing any behavior for end users, see #2413 (comment)
There was a problem hiding this comment.
The VM was previously calling _channel.sink.close() here. Since _gracefulShutdown is only needed to work around issues with Node.js, does this code need to change at all? Or could we just have the VM gracefully shut down even though it doesn't strictly need to, just to remove some extra complexity?
Either way, it's worth documenting those Node.js issues in the code itself so we know what to work around when making changes in the future.
lib/src/embedded/js/concurrency.dart
Outdated
There was a problem hiding this comment.
This can also just go in the existing io catch-all.
| if (_gracefulShutdown) { | ||
| _channel.sink.close(); | ||
| } else { | ||
| exit(exitCode); | ||
| } |
There was a problem hiding this comment.
The VM was previously calling _channel.sink.close() here. Since _gracefulShutdown is only needed to work around issues with Node.js, does this code need to change at all? Or could we just have the VM gracefully shut down even though it doesn't strictly need to, just to remove some extra complexity?
Either way, it's worth documenting those Node.js issues in the code itself so we know what to work around when making changes in the future.
lib/src/embedded/js/io.dart
Outdated
There was a problem hiding this comment.
This should be part of the existing IO library (which already has definitions for exitCode).
There was a problem hiding this comment.
Well, I was trying to implement this with new js interop from dart 3.3 without relying on the node_interop package which heavily depends on deprecated dart:js_util, which will start to break at some point.
For exitCode I can use the existing one we have.
What about Stdin, Stdout, Stderr? These are effectively wrapper classes on node stdio to create an interface that's same as the dart built-in classes while it only partially implement necessarily methods we need on those interface. @nex3 What's your preference here? Create another interface that wraps the original dart interface?
lib/src/embedded/js/js.dart
Outdated
There was a problem hiding this comment.
Any work to expose existing JS APIs should go in lib/src/js.
There was a problem hiding this comment.
Again, this is a saparate file because it's new js interop from dart 3.3, which I found a bit awkward to put it together with the classic interop code in the same place. Any recommendation on how to deal with it?
|
Applied most of the feedbacks. However, I have a awkward feeling about mixing classic js interop and new js interop in the same file, which is why I have lots of js specific things with new interop are only in embedded folder instead of in the generic folder. @nex3 I haven't changed these yet, as I want to hear what's your thought on this. |
|
Another note: The reason I tried to avoid classic interop and node_introp is that I couldn't get The new dart 3.3 js interop on the other hands was way more straight forward to use and debug. I got |
Closes #2325.
sass/embedded-host-node#344
Implementation
The actual isolate dispatcher and compilation dispatcher are nearly unchanged. However, I had to replace isolate with worker communication, and mock tons of small things that do not work on node.
Testing